ggml-cpu : split arch-specific implementations #13892
Conversation
|
I'll need a bit more time to get this PR rebased onto the latest master branch. |
github-actions
bot
added
the
ggml
|
@ggerganov @slaren |
ggml/src/ggml-cpu/ggml-cpu-impl.h
Outdated
|
|
||
| #define GGML_DO_PRAGMA_(x) _Pragma (#x) | ||
| #define GGML_DO_PRAGMA(x) GGML_DO_PRAGMA_(x) | ||
| #if defined(GGML_CPU_GENERIC) || defined(__HIPCC__) | ||
| // weak alias not working | ||
| # define GGML_WEAK_ALIAS(name, alias) | ||
| #elif defined(__GNUC__) | ||
| // GCC/Clang on *nix | ||
| # define GGML_WEAK_ALIAS(name, alias) GGML_DO_PRAGMA(weak name = alias) // NOLINT | ||
| #elif defined(_MSC_VER) && defined (_WIN64) | ||
| // MSVC | ||
| // Note: C name mangling varies across different calling conventions | ||
| // see https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170 | ||
| # define GGML_WEAK_ALIAS(name, alias) GGML_DO_PRAGMA(comment(linker, "/alternatename:" #name "=" #alias)) | ||
| #else | ||
| # error "Unsupported compiler for GGML_WEAK_ALIAS" | ||
| #endif | ||
|
|
||
| #define GGML_CPU_NATIVE_IMPL(name) GGML_WEAK_ALIAS(name, name ## _generic) |
There was a problem hiding this comment.
"generic" implementations such as ggml_vec_dot_q4_0_q8_0_generic are always defined, regardless of the build configuration. And by making these definitions "weak", we allow them to be overwritten by arch-specific implementation.
Is my understanding of this logic correct?
There was a problem hiding this comment.
Yes, on supported targets. The linker will first attempt to resolve implemented arch-specific symbols (like ggml_vec_dot_q4_0_q8_0). If an optimized version is not found, it automatically falls back to the _generic variants. I believe this approach offers improved maintainability at the cost of a slight increase in binary size due to these shadowed fallback implementations.
| #endif | ||
| for (; ib < nb; ++ib) { | ||
| int sumi0 = 0; | ||
| int sumi1 = 0; | ||
|
|
||
| for (int j = 0; j < qk/2; ++j) { | ||
| const int v0 = (x[ib].qs[j] & 0x0F) - 8; | ||
| const int v1 = (x[ib].qs[j] >> 4) - 8; | ||
|
|
||
| sumi0 += (v0 * y[ib].qs[j]); | ||
| sumi1 += (v1 * y[ib].qs[j + qk/2]); | ||
| } | ||
|
|
||
| int sumi = sumi0 + sumi1; | ||
| sumf += sumi*GGML_FP16_TO_FP32(x[ib].d)*GGML_FP16_TO_FP32(y[ib].d); | ||
| } | ||
|
|
||
| *s = sumf; | ||
| } |
There was a problem hiding this comment.
Currently, each arch re-implements the generic version. For this big refactoring this is probably the correct approach in order to minimize the risk of introducing bugs. But after we merge, we should consider ways to de-duplicate the scalar implementations by reusing the generic calls?
There was a problem hiding this comment.
That should be feasible, though it's not as straightforward for tail elements like the ones in this function. Let's keep the deduplication work for another PR to make future bisecting easier.
github-actions
bot
added
Nvidia GPU
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
This PR reworks the arch-specific code organization, following the discussion in #13720. Key changes include splitting the former
ggml-cpu-quants.candggml-cpu-aarch64.cppinto several more focused files. Additionally,aarch64-related naming and identifiers, originally specific to AArch64 but now applicable to other architectures, have been updated to userepackto improve clarity and avoid confusion.